Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 레디스 적용 및 로그아웃 api 추가 #40

Merged
merged 27 commits into from
Jul 11, 2024
Merged

Conversation

eom-tae-in
Copy link
Owner

@eom-tae-in eom-tae-in commented Jul 10, 2024

📄 Summary

  • 관리자 RT를 레디스에 저장하기 위해 레디스를 적용하였고 저장, 조회, 삭제 기능을 추가하였습니다.
  • 관리자 로그아웃 기능을 추가하였습니다. 로그인한 유저가 로그아웃을 하게되면 저장되어있던 RT가 필요가 없기 때문에 불필요한 DB 낭비가 생기게 되어 로그아웃 기능을 개발하게 되었습니다.

close #38

Copy link
Collaborator

@devholic22 devholic22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다! 몇 가지만 확인해주시면 될 것 같습니다 :)

@@ -24,9 +26,12 @@

@NoArgsConstructor
@Component
public class AdminJwtTokenProvider implements AdminTokenProvider {
public class AdminJwtAccessTokenProvider implements AdminAccessTokenProvider,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적인 생각으로는 해당 클래스에서 AT와 RT를 모두 만들고 있기 때문에 기존 이름 (AdminJwtTokenProvider)이 더 적합할 것 같은데, 클래스 이름을 변경하신 이유가 있으실까요?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JwtTokenProvider 구조를 변경하면서 이름이 자동으로 변경되었던 것 같습니다 수정하도록 하겠습니다!

@@ -56,6 +57,14 @@ public ResponseEntity<AdminAccessTokenResponse> reGenerateAccessToken(
return ResponseEntity.ok(adminAuthService.reGenerateAccessToken(refreshToken));
}

@DeleteMapping("/logout")
public ResponseEntity<Void> login(@AdminRefreshToken final String refreshToken) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그아웃을 하는 메서드인만큼 메서드명을 logout으로 하는 게 더 좋을 것 같습니다!

@Autowired
private AdminRedisRefreshTokenRepository adminRedisRefreshTokenRepository;


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공백 제거해주시면 될 것 같습니다!


private final RedisTemplate<String, Long> redisTemplate;


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공백 있습니다 :)


@Builder
public record AdminRefreshToken(
@Id String refreshToken,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Id가 들어간 이유가 궁금한데, 혹시 고유 식별자 역할로 쓰이게 하시기 위함일까요?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

레디스에 사용할 도메인 객체에 키로 사용되는 값을 지정해야 하는 걸로 알고 있었는데 로컬에서 제거해서 테스트를 진행해도 잘 되네요! @id를 지정해줘야되는지 알고 사용한 이유 외에 다른 이유는 없었습니다. @id는 Redis Repository로 구현했을 때만 사용하는 게 맞을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인해보니 레디스로 관리할 경우에는 말씀하신 것 처럼 @Id (springframework.data의 어노테이션)가 필요한 것으로 나오네요..! 그대로 남겨두시는게 맞지 않을까 생각됩니다. 참고하실만한 공유드리겠습니다!

@@ -0,0 +1,6 @@
package com.atwoz.admin.domain.admin.service;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AT 발급 인터페이스는 application/auth, RT 발급 인터페이스는 domain/admin/service에 작성하신 이유가 궁금합니다. 궁극적으로 모두 토큰을 발급하기도 하고, 구현체 또한 AdminJwtAccessTokenProvider에서 모두 구현한만큼 이를 추상화한 인터페이스들의 위치도 통일시키는 게 좋을 것 같은데 어떻게 생각하시나요?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저희가 RT만 레디스에 저장하기 때문에 RT 도메인 객체는 도메인에 위치해야합니다. 하지만 RT의 암호화된 토큰은 JwtTokenProvider에서 만들어집니다. 만약 JwtTokenProvider 통해 서비스 레이어에서 생성해준다면 RT 도메인 객체는 응용 계층에서 만든 암호화된 토큰을 받게 됩니다. 이로 인한 응용 계층의 의존이 생기기 때문에 RTProvider를 팩토리 메서드 인자로 넘겨줘서 내부에서 암호화된 토큰이 만들어지도록 구현해야한다고 생각했습니다. RTProvider는 도메인 서비스이기에 도메인 레이어에 위치해야합니다. 하지만 ATProvider는 도메인에 어떠한 영향도 미치지 않기 때문에 응용 계층에서 생성해서 응답 바디에 넘겨주면 됩니다. 제가 AT, RT 생성 기능을 모두 추상화한 인터페이스로 둔 이유가 바로 이 이유 때문이었습니다. 추가적인 의견 및 제가 잘못 이해해서 설계한 부분이 있다면 댓글로 의견 남겨주시면 감사하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 이해했습니다. AT 발급 (ATProvider)은 도메인에 영향을 미치지 않고 응용 계층에서만 이용되기 때문에 응용 계층에 인터페이스를 위치시키고, RT 발급 (RTPRovider)을 AT 발급처럼 응용 계층에 둔다면 도메인 계층에 있는 AdminRefreshToken에서 응용 계층에 대한 의존이 발생되기 때문에 도메인 패키지 하위에 서비스 패키지를 별도로 만들어주신거군요.
네 저도 이 방식이 더 나은 방식 같습니다! 태인님의 의견을 들으면서 인터페이스를 둘 위치를 더 깊게 고민할 수 있어서 좋았습니다 :)

@@ -33,37 +36,46 @@ public AdminTokenResponse signUp(final AdminSignUpRequest adminSignUpRequest) {
adminProfileSignUpRequest.phoneNumber()
);
Admin savedAdmin = adminRepository.save(admin);
AdminRefreshToken adminRefreshToken = createAdminRefreshToken(savedAdmin);
adminRefreshTokenRepository.save(adminRefreshToken);
String accessToken = adminAccessTokenProvider.createAccessToken(savedAdmin.getId());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AT를 만들기까지의 과정과 RT를 만들기까지의 과정을 메서드 분리하는 것은 어떨지 여쭤보고 싶습니다..! (RT의 경우, 리포지터리에 save하는 것도 createAdminRefreshToken 메서드에 포함시키면 될 것 같습니다.)

또는, Admin을 save하는 것을 메서드 분리해도 좋을 듯 합니다.

public AdminTokenResponse signUp(final AdminSignUpRequest adminSignUpRequest) {
    Admin savedAdmin = saveAdmin(adminSignUpRequest);
    String accessToken = adminAccessTokenProvider.createAccessToken(savedAdmin.getId());
    AdminRefreshToken adminRefreshToken = createAdminRefreshToken(savedAdmin);

    return new AdminTokenResponse(accessToken, adminRefreshToken.refreshToken());
}

private Admin saveAdmin(final AdminSignUpRequest request) {
    AdminProfileSignUpRequest adminProfileSignUpRequest = request.adminProfileSignUpRequest();
    Admin admin = Admin.createWith(
        request.email(),
        request.password(),
        request.confirmPassword(),
        adminProfileSignUpRequest.name(),
        adminProfileSignUpRequest.phoneNumber()
    );
    return adminRepository.save(admin);
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 분리를 굳이 할 필요가 없어보여 하지 않았는데 다른 곳에서 재사용을 하지 않아도 작성하신 코드를 보니 분리하는게 나을 것 같다는 생각이 드네요. 메서드 분리하도록 하겠습니다!

Copy link
Collaborator

@devholic22 devholic22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다!

Copy link
Collaborator

@sosow0212 sosow0212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다 👍 계층 구조 잘 나눠주신 것 같아서 코드리뷰 하기도 너무 편했습니다!
타임아웃도 잘 설정해주시고 딱히 변경해야할 부분이 보이진 않아서 Approve 합니다!

Comment on lines +82 to 84
public void logout(final String refreshToken) {
adminRefreshTokenRepository.delete(refreshToken);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프론트엔드에서 헤더에서 토큰을 제거해주고 동시에 로그아웃 api를 요청해서 리프레쉬 토큰까지 제거해서 로그아웃을 시켜준다라는 의미로 작성하신 코드가 맞을까요?!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 맞습니다!
프론트에서 로그아웃 처리시 로그아웃 api를 요청해서 서버에 있는 로그인 했던 유저의 RT를 삭제합니다!

@eom-tae-in eom-tae-in merged commit 8f51049 into develop Jul 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

레디스 적용 및 관리자 인증 기능 수정
3 participants